Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TP2000-1478: Missing measures check #1359

Merged
merged 17 commits into from
Jan 16, 2025

Conversation

eadpearce
Copy link
Contributor

@eadpearce eadpearce commented Dec 13, 2024

TP2000-1478: Missing measures check

Why

  • To reduce errors in workbaskets by adding a check to ensure every commodity code has at minimum a type 103 measure associated (Most Favoured Nation or MFN)

What

  • Adds new tab layout design for workbasket checks
  • New page where users can run the missing measures check and see results
  • New database models MissingMeasuresCheck and MissingMeasureCommCode added
  • Extra field added to WorkBasket model - missing_measures_check_task_id. Functions the same as rule_check_task_id
  • The check blocks the publishing of the workbasket if it doesn't pass, like the business rules check
  • The check runs asynchronously in a celery task
  • The check can be stopped and re-run

Still to do

  • Add green and red boxes to match latest design and business rules check page

Checklist

  • Requires migrations? Yes
  • Requires dependency updates? No
Screenshot 2024-12-19 at 12 13 24 Screenshot 2024-12-19 at 12 13 37 Screenshot 2024-12-19 at 12 14 31

@eadpearce eadpearce marked this pull request as ready for review December 18, 2024 14:06
@eadpearce eadpearce requested a review from a team as a code owner December 18, 2024 14:07
Copy link
Collaborator

@paulpepper-trade paulpepper-trade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some screen shots of views and forms in the PR cover note could be useful.

@eadpearce eadpearce force-pushed the TP2000-1478-missing-measures-check branch from b59c4ee to 73ee57a Compare January 7, 2025 14:55
return super().get_queryset()

@atomic
def run_missing_measures_check(self, pks):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhap self-document this function?

Suggested change
def run_missing_measures_check(self, pks):
def run_missing_measures_check(self, comm_code_pks: list[int]) -> None:

Comment on lines +161 to +187
class MissingMeasuresCheck(TimestampedMixin):
"""Stores a timestamp for when check_workbasket_for_missing_measures was
last run, FK to the workbasket and whether the check was successful."""

workbasket = models.OneToOneField(
"workbaskets.WorkBasket",
on_delete=models.CASCADE,
null=True,
related_name="missing_measures_check",
)

successful = fields.BooleanField(null=True)


class MissingMeasureCommCode(models.Model):
"""Links a GoodsNomenclature to a MissingMeasuresCheck when the commodity
fails the check."""

commodity = models.ForeignKey(
"commodities.GoodsNomenclature",
on_delete=models.CASCADE,
)
missing_measures_check = models.ForeignKey(
MissingMeasuresCheck,
on_delete=models.CASCADE,
related_name="model_checks",
)
Copy link
Collaborator

@paulpepper-trade paulpepper-trade Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to take a different approach to business rule checks.

Business rule checks have a matching TrackedModelCheck.successful=True and new timestamp for each WorkBasket TrackedModel when all business rules have passed. But if there's an incomplete set of TrackedModelChecks, or one or more with a successful attribute set to False, or timestamp older than TrackedModel.updated_at , then business rules are considered to have failed.

Goods are unlikely to change in a workbasket. But measures may do, which would make a clean MissingMeasuresCheck instance invalid. Is staleness and check completeness validation required or supported in this implementation?

Either way, a docstring would be useful, perhaps in MissingMeasuresCheck, to say a little about the checking strategy because of the differing approaches.

@eadpearce eadpearce merged commit 9165647 into master Jan 16, 2025
8 checks passed
@eadpearce eadpearce deleted the TP2000-1478-missing-measures-check branch January 16, 2025 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants